Add WDL implementation for Salmon by PriyankaaXD · Pull Request #326 · stjudecloud/workflows

PriyankaaXD · 2026-08-13T00:19:59Z

Adds a WDL implementation for Salmon (mapping-mode only), per the "tool wishlist" issue #228.

Tasks added in tools/salmon.wdl:

build_salmon_index — wraps salmon index
quant — wraps salmon quant

All "important options" from Salmon's docs are exposed as inputs, with defaults verified against salmon quant --help-reads output on Salmon 1.9.0. Parameter documentation is copied from Salmon's official docs, per guidance in the issue. Scoped to mapping-mode only (FASTQ input) — no BAM/alignment-mode support, as requested.

Tests added in tools/salmon.yml using the new Sprocket test framework, covering both tasks with real output assertions. Verified locally: sprocket lint passes cleanly, sprocket dev test passes both tests.

This is my first contribution to this project — happy to make any adjustments you'd like!

Before submitting this PR, please make sure:

You have added a few sentences describing the PR here.
The code passes all CI tests without any errors or warnings.
You have added tests (when appropriate).
You have added an entry in any relevant CHANGELOGs (when appropriate).
If you have made any changes to the scripts/ or docker/ directories, please ensure any image versions have been incremented accordingly!
You have updated the README or other documentation to account for these changes.

stjudecloud-cloudy · 2026-08-13T00:20:17Z

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

adthrasher · 2026-08-13T13:16:57Z

I didn't check out the commit, but can you comment on where these reads were sourced? We want to track the origin our our test data.

I'd prefer if we didn't add these files. We already have FASTQ fixtures - https://github.com/stjudecloud/workflows/blob/main/test/fixtures/fastqs/README.md

The existing test fixtures should be reused (re: #280 , I don't want more LFS files hitting the history )

I removed these customs files entirely and switched to reusing the existing shared fixtures (fastqs/test_R1.fq.gz/test_R2.fq.gz) instead and rebuilt the test transcriptome from real sequences within those files, so both build_salmon_index and quant tests now run against existing shared data rather than anything new.

adthrasher · 2026-08-13T13:18:57Z

Can you rename this to use yaml as the extension to match our repository convention?

Also this should go under tools/test/.

adthrasher · 2026-08-13T13:20:54Z

+
+    runtime {
+        cpu: ncpu
+        memory: "16 GB"


Does salmon use a consistent amount of RAM or is it dependent on the input and/or transcriptome?

Sorry, I originally hardcoded 16 GB without thinking much about how the memory usage would actually vary.

Salmon's memory usage mainly depends on the index size, which depends on the transcriptome size and whether decoys are included. The input reads are streamed, so their size doesn't have as much impact on RAM usage.

I've now updated both build_salmon_index and quant to calculate memory_gb dynamically based on the input size, similar to how disk_size_gb is already handled. There's also a modify_memory_gb option if the estimate needs to be adjusted for specific data.

adthrasher · 2026-08-13T13:22:38Z

+        cpu: ncpu
+        memory: "16 GB"
+        disks: "~{disk_size_gb} GB"
+        container: "quay.io/biocontainers/salmon:1.9.0--h7e5ed60_0"


Is there a reason we're using such an old version of salmon?

I'd picked 1.9.0 somewhat arbitrarily. I did try updating to 1.12.1 (the latest release still on the original C++ codebase — 2.0+ is a full Rust rewrite with a different index format, so I avoided that for now), but that specific container build (quay.io/biocontainers/salmon:1.12.1--h017bda4_0) hits a locale::facet::_S_create_c_locale crash during indexing in my test environment — a known class of bug in minimal Docker images missing locale data, unrelated to our WDL logic itself. Reverting to 1.9.0, which runs cleanly and passes both tests. Happy to revisit if you know of a working newer tag, or if this is worth filing upstream with BioContainers.

according to the official migration doc - https://github.com/COMBINE-lab/salmon/blob/master/MIGRATION.md

I'm not seeing any reason we wouldn't want the latest version (rewrite and all). @adthrasher any reason you see not to use a >=v2 version?

adthrasher · 2026-08-13T13:24:06Z

+            "~{if length(read_twos) == 0 then "--fldMean " + fld_mean else ""}" \
+            "~{if length(read_twos) == 0 then "--fldSD " + fld_sd else ""}" \


I don't think this will work as the arguments end up quoted in bash. Was this an attempt to address a sprocket lint warning?

adthrasher · 2026-08-13T13:28:16Z

@@ -0,0 +1,255 @@
+version 1.1
+
+task build_salmon_index {


The build task likely needs the decoys mode exposed as that is the recommended way to run with mapping mode.

The information on running with decoys is scattered and not mentioned on the 2.0 doc site - https://combine-lab.github.io/salmon/

I was able to find this on the old docs site - https://salmon.readthedocs.io/en/latest/salmon.html#preparing-transcriptome-indices-mapping-based-mode
and this SC guide that seems to be where @PriyankaaXD pulled the current Bash from - https://combine-lab.github.io/alevin-tutorial/2019/selective-alignment/

I think decoy-aware indexing will be a must for any production workflow, but it seems like a barrel of worms that we can address in a follow up PR. I think we'll probably need a separate WDL task for doing the decay-aware ref building, and for this PR we can merge without touching decoys at all.

Unless @adthrasher is there a straightforward solution I'm missing?

adthrasher · 2026-08-13T13:29:17Z

+            -l "~{lib_type}" \
+            -1 ~{sep(" ", squote(read_one_fastqs_gz))} \
+            ~{if length(read_twos) > 0 then "-2 " + sep(" ", squote(read_twos)) else ""} \
+            --validateMappings \


This is the default right? This probably needs to be a Boolean input with a true default.

adthrasher · 2026-08-13T13:29:55Z

+          - Name: salmon_index.tar.gz
+
+quant:
+  - name: quantifies_paired_end_reads


Since SE mode is implemented, it should get a test.

I added a single-end test — and it actually caught a real bug: quant was always using -1/-2 regardless of read type, which Salmon rejects for genuine single-end input (it requires -r instead). Fixed the command logic to switch based on whether read_two_fastqs_gz is provided.

adthrasher · 2026-08-13T13:30:30Z

The index here also needs documentation on how it was generated.

adthrasher · 2026-08-13T13:31:21Z

+
+    input {
+        File salmon_index_tar_gz
+        Array[File] read_one_fastqs_gz


Suggested change

Array[File] read_one_fastqs_gz

Array[File]+ read_one_fastqs_gz

This needs to be non-empty.

@adthrasher I think we stopped using non-empty arrays as the resulting WDL is unwieldy ? Or we had a commit adding them and then removing them? I can't remember where we landed on it, but I'm fine without this. If the user doesn't supply any FASTQs, salmon will blow up with an informative error, so 🤷‍♀️

I can't remember the details now. It's something we should probably revisit, though. I'd much rather the WDL fail upfront at analysis because of an empty array than the underlying tool erroring. If WDL doesn't do non-empty arrays well, then we should push for updates to the spec and to the engine(s).

I'll investigate 🫡
in the mean time, let's follow through with Andrew's original recommendation here and make it non-empty

The CHANGELOG entry points to #240

adthrasher · 2026-08-13T13:32:06Z

This also needs a CHANGELOG entry.

…d FASTQ data, add SE test

PriyankaaXD added 2 commits August 2, 2026 18:32

Update Salmon container image to a more current version

c88526a

Add Sprocket unit tests for Salmon WDL tasks

eedc21a

PriyankaaXD mentioned this pull request Aug 13, 2026

tool wishlist #228

Open

adthrasher requested review from DelaramR, a-frantz and adthrasher August 13, 2026 13:14

adthrasher assigned PriyankaaXD Aug 13, 2026

adthrasher requested changes Aug 13, 2026

View reviewed changes

PriyankaaXD added 7 commits August 17, 2026 08:48

Fix fldMean/fldSD argument quoting and suppress SC2086 false-positive

d388086

Move test file to tools/test/ and rename to .yaml extension

01cfcb1

Fix single-end quant mode (-r flag), rebuild test fixtures from share…

cb2894d

…d FASTQ data, add SE test

Require non-empty array for read_one_fastqs_gz

634d615

Expose validateMappings as a Boolean input

a94ad22

Expose decoy-aware indexing as an optional input on build_salmon_index

4405eb0

Make memory allocation dynamic for both tasks, based on input size

ad33e26

		"~{if length(read_twos) == 0 then "--fldMean " + fld_mean else ""}" \
		"~{if length(read_twos) == 0 then "--fldSD " + fld_sd else ""}" \

	Array[File] read_one_fastqs_gz
	Array[File]+ read_one_fastqs_gz

Uh oh!

Conversation

PriyankaaXD commented Aug 13, 2026

Uh oh!

stjudecloud-cloudy commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Snyk checks have passed. No issues have been found so far.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adthrasher commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stjudecloud-cloudy commented Aug 13, 2026 •

edited

Loading